Skip to content
This repository has been archived by the owner on Apr 20, 2019. It is now read-only.

Inject credentials from batch endpoint if applicable #87

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alphapseudo
Copy link

This pull request serves as an optimization for batch requests that require authentication.

Expected Result:
If the batch endpoint requires authentication, it should re-use those credentials for any of the subsequent injected requests. In other words, it should only check the session storage once for all the requests in a batch.

Actual Result:
The subsequent requests ignore the initial credential validation and performs a session lookup for the n requests.

Reproduction Steps:

  1. Apply an authentication strategy to your batch endpoint (e.g.)
    options: {
      batchEndpoint: '/api/batch',
      tags: ['batch'],
      auth: 'session'
    }
  1. Define an endpoint that requires the same authentication strategy (e.g.)
  {
    method: 'POST',
    path: '/api/users',
    config: {
      auth: {
        strategy: 'session',
        scope: ['users:read']
      }
    },
    handler: UserController.createUser
  }
  1. Make a POST request with a payload (e.g.)
[ { method: 'POST',
    path: '/api/users',
    payload: { first_name: 'Andrew', last_name: 'Fake' } },
  { method: 'POST',
    path: '/api/users',
    payload: { first_name: 'John', last_name: 'Doe' } } ]
  1. Observe session storage. The session lookup is linear to the number requests instead of constant.

Copy link
Contributor

@cadecairos cadecairos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alphapseudo, Thanks for submitting this pull request!

Before this can get merged, can you please write a unit test for this change?

@hueniverse
Copy link
Contributor

@cadecairos looks like @alphapseudo moved on. Either finish the work yourself or close the issue.

@alphapseudo
Copy link
Author

@cadecairos sorry about the delay, I added a test in fb9da91

@hueniverse
Copy link
Contributor

@cadecairos Can we bring this to a conclusion?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants